Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surefire/Failsafe plugin configuration propagated to Junit/TestNG launch configuration #1672

Conversation

treilhes
Copy link
Contributor

The following arguments are supported:

<argLine>,

<environmentVariables>,

<systemPropertyVariables>,

<workingDirectory>,

<enableAssertions>,

Configuration is propagated on unit test launch configuration creation and also when executing maven > update project

By default and if found, the plugin maven-dependency-plugin (goal: properties) is executed before updating the launch configuration to load properties.

If properties set by other plugins are used in the failsafe/surefire plugin configuration it is possible to override the default loading behaviour by providing the list plugins and goals to execute using the property m2e.launch.configuration.prerequisites

The expected format is as follow:

groupId1:artifactId1:goal1[,groupIdX:artifactIdX:goalX]*

Ex:
<m2e.launch.configuration.prerequisites>org.apache.maven.plugins:maven-dependency-plugin:properties,org.codehaus.mojo:properties-maven-plugin:read-project-properties</m2e.launch.configuration.prerequisites>

@treilhes
Copy link
Contributor Author

treilhes commented Feb 13, 2024

Fixes #700

Copy link

github-actions bot commented Feb 13, 2024

Test Results

  216 files  + 2    216 suites  +2   20m 1s ⏱️ + 3m 0s
  673 tests + 8    654 ✅ ±0  18 💤 + 8  1 ❌ ±0 
1 346 runs  +16  1 309 ✅ ±0  36 💤 +16  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 5763fc6. ± Comparison against base commit a4387fc.

♻️ This comment has been updated with latest results.

@treilhes treilhes mentioned this pull request Feb 14, 2024
@HannesWell
Copy link
Contributor

@treilhes thank you for this contribution. In general it is interesting and definitely a good thing to support.

Unfortunately I won't have time to review this large contribution over the weekend and will then be on vacation for three weeks.
So unless another committer reviews this your patience is kindly requested.

@treilhes
Copy link
Contributor Author

@HannesWell
Don't worry, my patience will handle this just fine. Have great holidays. thank you for having warned

@treilhes
Copy link
Contributor Author

@HannesWell, I miss you so much, I long to hear from you about your vacation (and to get a bit of review) 💔

@HannesWell
Copy link
Contributor

@HannesWell, I miss you so much, I long to hear from you about your vacation (and to get a bit of review) 💔

My vacation was great, thanks for asking. I'll do my best to review this at the weekend (when I arrived back home a full ToDo-List hit immediately).

@treilhes
Copy link
Contributor Author

@HannesWell Bump ❤️

Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @treilhes first of all a big sorry for the delay and thanks for your patience.
It's just currently a lot to do, but I still have you on my TODO list a will try my best that we can complete this before the next release (ideally with some time left).

I had a a very brief overview over this change and I find your solution to adjust the launch config very interesting. I assume this works reliably in various scenarios like creating via a short-cut or modifying an existing config? If all works as expected that's a smart solution.
I'm asking because having a mechanism for that is a longer standing issue:

Overall I didn't see any blockers and I'm glad you added extensive tests.

Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made another pass over this PR and have a couple of suggestions, mainly about reducing code size.

@treilhes treilhes requested a review from HannesWell June 28, 2024 19:56
@HannesWell HannesWell force-pushed the feature/surefire_plugins_arguments_to_launch_config branch from 85966cd to afc83e4 Compare August 25, 2024 22:28
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@treilhes thanks for your adjustments and your patience.
I applied multiple changes to stream-line this PR a bit but it should not have changed anything from a functional point of view.

With that this looks fine to me and I would like to integrate this into the M2E release for the upcoming Eclipse simultaneous 2024-09 release.
If you are fine with these changes I'll squash them into one commit and submit it.

I just have one minor question below.

Comment on lines 236 to 239
boolean isMavenProject = configuration
.getAttribute(IJavaLaunchConfigurationConstants.ATTR_CLASSPATH_PROVIDER, "")
.equals(MavenRuntimeClasspathProvider.MAVEN_CLASSPATH_PROVIDER);
//TODO: why not check the nature?!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be sufficient to just test if the configuration's project has the m2e/maven nature, which is done below?

@HannesWell HannesWell force-pushed the feature/surefire_plugins_arguments_to_launch_config branch from 6aabd05 to 4f57779 Compare August 27, 2024 20:22
@HannesWell HannesWell force-pushed the feature/surefire_plugins_arguments_to_launch_config branch from 4f57779 to 5763fc6 Compare August 27, 2024 20:23
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@treilhes thank you for this contribution, that's really a nice extension to m2e and good work.

I'll leave the last question open for now because I would like to have this in the RC1 for the upcoming m2e release that I'm currently preparing.
We can still adjust this later.

@HannesWell HannesWell merged commit c8bcaaa into eclipse-m2e:master Aug 27, 2024
6 of 7 checks passed
@treilhes
Copy link
Contributor Author

Hello @HannesWell,
Replacing the MAVEN_CLASSPATH_PROVIDER existence check by a maven nature check is totaly valid.
If i remember well the classpath check come from the ModuleSupport class and it was a lazy copy/paste.
It seems this check was also replaced by a maven nature check as i can't find it anymore.

@HannesWell
Copy link
Contributor

Thank you for the clarification.
Do you want to provide a follow-up to remove it?

@treilhes
Copy link
Contributor Author

Hello @HannesWell,
You can find the fix in #1818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants